Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: fix http-upgrade-agent flakiness #4520

Closed
wants to merge 1 commit into from
Closed

test: fix http-upgrade-agent flakiness #4520

wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the data event to make sure all
the data is received. Modify the test so it does not pass without
applying this change.

I was getting this error from time to time on OS X:

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: <Buffer > == 'nurtzo'
    at ClientRequest.<anonymous> (/Users/sgimeno/node/node/test/parallel/test-http-upgrade-agent.js:51:12)
    at emitThree (events.js:98:13)
    at ClientRequest.emit (events.js:176:7)
    at Socket.socketOnData (_http_client.js:342:11)
    at emitOne (events.js:78:13)
    at Socket.emit (events.js:170:7)
    at readableAddChunk (_stream_readable.js:146:16)
    at Socket.Readable.push (_stream_readable.js:110:10)
    at TCP.onread (net.js:523:20)

@Trott
Copy link
Member

Trott commented Jan 3, 2016

Modify the test so it does not pass without applying this change.

This PR only has a change to the test. Is this sentence in error or is there a missing modified file that needs to be added?

@brendanashworth brendanashworth added the test Issues and PRs related to the tests. label Jan 4, 2016
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jan 4, 2016
@santigimeno
Copy link
Member Author

Yes, that sentence was wrong. I rewrote the commit message. PTAL

@Trott
Copy link
Member

Trott commented Jan 8, 2016

The changes to the upgrade event handler looks good to me. I don't think we need the setTimeout() in the test setup. If you do want to force the data in two writes like that, maybe put it in a different test so we have one test that's straightforward and another test that's specific to the data definitely always arriving in two pieces?

@santigimeno
Copy link
Member Author

@Trott PR updated with your comments. Thanks

@Trott
Copy link
Member

Trott commented Jan 9, 2016

LGTM if CI is happy.

CI: https://ci.nodejs.org/job/node-test-commit/1664/

We need to get you onboarded so you can start these CI jobs yourself!

Minor nit that can be ignored if you want: I don't think we need the gotData variable and check in the new test since we're guaranteeing the check of the value of recvData which can't possibly be set correctly unless gotData is true. But it doesn't hurt anything either, I guess.

@Trott
Copy link
Member

Trott commented Jan 9, 2016

One other comment that you can ignore if you want: The same issue you're fixing here seems to afflict test-http-upgrade-client.js as well. Maybe fix that too, either here or in a separate PR? (I'm a fan of separate PR, but either way.)

@santigimeno
Copy link
Member Author

@Trott I've removed the gotData variable.
I'll fix the other test in a separate PR

@Trott
Copy link
Member

Trott commented Jan 9, 2016

Looks like the modified test fails (or is flaky?) on FreeBSD and Raspberry Pi:

@santigimeno
Copy link
Member Author

@Trott It seems the CI was started with the previous version of the PR: 8ac8915 (the one with gotData) and not the current one: santigimeno@d710d9c. Anyway, it's been helpful as it has shown that in those environments all the upgrade data was sent in only 1 chunk. I'll increase the timeout a little bit and maybe bring back the gotData variable as it has proven to be useful. What do you think?

@santigimeno
Copy link
Member Author

Nah, increasing the timeout won't work either as the client is closing the connection when it receives the upgrade request. I think I'm removing the test altogether as it may happen that the second write could fail due to a race condition with the client closing the connection.

@santigimeno
Copy link
Member Author

PR updated. I think this should work

@Trott
Copy link
Member

Trott commented Jan 10, 2016

One nit, but LGTM if CI doesn't uncover anything.

CI: https://ci.nodejs.org/job/node-test-pull-request/1180/

Trott
Trott reviewed Jan 10, 2016
View reviewed changes
test/parallel/test-http-upgrade-agent.js
recvData += d;
});

socket.on('close', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: As mscdex noted in the other PR that's similar to this, this callback might benefit from being wrapped in common.mustCall() to guarantee that it fires.

@santigimeno
test: fix http-upgrade-agent flakiness
6e84dda 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.
@santigimeno
Copy link
Member Author

PR updated. Thanks

@Trott
Copy link
Member

Trott commented Jan 11, 2016

LGTM.

CI: https://ci.nodejs.org/job/node-test-pull-request/1200/

@jasnell
Copy link
Member

jasnell commented Jan 12, 2016

LGTM

jasnell pushed a commit that referenced this pull request Jan 12, 2016
@santigimeno @jasnell
test: fix http-upgrade-agent flakiness
6018fa1 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 12, 2016

Landed in 6018fa1

@jasnell jasnell closed this Jan 12, 2016
rvagg pushed a commit that referenced this pull request Jan 14, 2016
@santigimeno @rvagg
test: fix http-upgrade-agent flakiness
afa4c1a 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request Jan 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
@santigimeno
test: fix http-upgrade-agent flakiness
06f118f 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
@santigimeno
test: fix http-upgrade-agent flakiness
6ef981e 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
@santigimeno
test: fix http-upgrade-agent flakiness
f10c937 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: nodejs#4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
@santigimeno
test: fix http-upgrade-agent flakiness
3d5bc69 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: nodejs#4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@santigimeno
test: fix http-upgrade-agent flakiness
046cfc4 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: nodejs#4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@santigimeno @Trott @jasnell @mscdex @MylesBorins @brendanashworth